Skip to content

Adding optional argument to create_child #515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: abstract-popmember
Choose a base branch
from
Open

Adding optional argument to create_child #515

wants to merge 10 commits into from

Conversation

@atharvas
Copy link
Contributor

@atharvas atharvas commented Oct 13, 2025 •
edited
Loading

Includes the addition of a mutation_choice to create_child since in many cases the child creation is dependent on the type of mutation (e.g. if we are tracking the number of llm calls, we need to see if mutation_choice was an llm_{choice})

One point that needs further discussion: Do we want to generalize this pattern? For my use case in LaSR all I need is the mutation_choice but we might want to generalize this. I'm not sure what would be the correct API to expose for this?

@atharvas atharvas changed the title New additions to abtract popmember to ensure LaSR compatibility. Adding optional argument to create_child Oct 13, 2025
@atharvas atharvas mentioned this pull request Oct 13, 2025
Open
@MilesCranmer
Copy link
Owner

MilesCranmer commented Oct 17, 2025

Thanks! I will think more about the right API.

@codex review

@chatgpt-codex-connector

This comment was marked as resolved.

Sign in to view
@atharvas
Copy link
Contributor Author

atharvas commented Oct 17, 2025

The new _parse_guesses_impl now constructs members of an arbitrary P<:AbstractPopMember, but the very next step still calls strip_metadata(member, ...). strip_metadata only has methods for PopMember, so if the user supplies a custom popmember_type this call will hit a MethodError before guesses can be processed (same issue will surface in other call sites that strip metadata). Consider adding an overload of strip_metadata for AbstractPopMember (using create_child or similar) so custom population members can survive the warm‑start path.

looking into this... Seems like I'll need to make a new constructor for subtypes of AbstractPopMember

@atharvas
Copy link
Contributor Author

atharvas commented Oct 17, 2025

Ok. Running all test cases locally right now but this passes the test_abstract_popmember.jl test suite.

@atharvas
Copy link
Contributor Author

atharvas commented Oct 17, 2025

Ok. Test cases pass locally. I can't get the JET tests to work properly. Any ideas why this happens?

ERROR: LoadError: failed process: Process(`/home/asehgal/.julia/juliaup/julia-1.11.7+0.x64.linux.gnu/bin/julia -C native -J/home/asehgal/.julia/juliaup/julia-1.11.7+0.x64.linux.gnu/lib/julia/sys.so --check-bounds=yes -g1 --startup-file=no --startup-file=no /home/asehgal/lasr-experiments/SymbolicRegression.jl/test/test_jet.jl`, ProcessExited(1)) [1]

Stacktrace:
  [1] pipeline_error
    @ ./process.jl:598 [inlined]
  [2] run(::Cmd; wait::Bool)
    @ Base ./process.jl:513
  [3] run(::Cmd)
    @ Base ./process.jl:510
  [4] top-level scope
    @ ~/lasr-experiments/SymbolicRegression.jl/test/runtests.jl:187
  [5] eval
    @ ./boot.jl:430 [inlined]
  [6] include_string(mapexpr::typeof(identity), mod::Module, code::String, filename::String)
    @ Base ./loading.jl:2734
  [7] include_string(m::Module, txt::String, fname::String)
    @ Base ./loading.jl:2744
  [8] #invokelatest#2
    @ ./essentials.jl:1055 [inlined]
  [9] invokelatest
    @ ./essentials.jl:1052 [inlined]
 [10] (::TestItemServer.var"#9#13"{String, Nothing, Vector{TestItemServer.CoverageTools.FileCoverage}})()
    @ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:352
 [11] withpath(f::TestItemServer.var"#9#13"{String, Nothing, Vector{TestItemServer.CoverageTools.FileCoverage}}, path::String)
    @ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:61
 [12] run_testitem(endpoint::TestItemServer.JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint}, params::TestItemServer.TestItemServerProtocol.RunTestItem, mode::String, coverage_root_uris::Nothing, state::TestItemServer.TestProcessState{var"#1#2"})
    @ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:344
 [13] runner_loop(state::TestItemServer.TestProcessState{var"#1#2"})
    @ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:658
 [14] (::TestItemServer.var"#33#35"{TestItemServer.TestProcessState{var"#1#2"}})()
    @ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:697
in expression starting at /home/asehgal/lasr-experiments/SymbolicRegression.jl/test/runtests.jl:187

@MilesCranmer
Copy link
Owner

MilesCranmer commented Oct 18, 2025

Sorry I tried to mark that codex review as "resolved" because I disagreed with it, my bad. But we actually want to keep strip_metadata specific to PopMember as a way to force downstream types to reimplement it. It's a bit safer than trying to come up with some automatic generic approach

@atharvas
Copy link
Contributor Author

atharvas commented Oct 18, 2025

Ah gotcha. Let me revert the changes and add it to the docs.

@atharvas
Copy link
Contributor Author

atharvas commented Oct 18, 2025

ok done. passes the test cases as well!

@MilesCranmer MilesCranmer mentioned this pull request Oct 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@atharvas @MilesCranmer